rework applying closure requirements in borrowck #102618

aliemjay · 2022-10-03T12:10:13Z

Previously the promoted closure constraints were registered under the category ConstraintCategory::ClosureBounds in type_check::prove_closure_bounds() and then mapped back their original category in regions_infer::best_blame_constraint using the complicated map closure_bounds_mapping.

Now we're registering promoted constraints under their original category and span earlier in type_check::prove_closure_bounds.

See commit messages.

Fixes #99245

rust-highfive · 2022-10-03T12:10:16Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

src/test/ui/generic-associated-types/issue-91139.rs

aliemjay · 2022-10-08T15:04:56Z

src/test/ui/nll/ty-outlives/projection-two-region-trait-bound-closure.stderr

-   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^ closure body requires that `'b` must outlive `'a`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ argument requires that `'b` must outlive `'a`


This is another minor diagnostic regression, but I believe it never was an intended behavior of the previous setup. Actually at the point of rendering diagnostics the intention was to not have a constraint with ClosureBounds category, but here we've somehow missed converting ClosureBounds constraints.

Anyway this regression only affect the niche case where there is double lifetime bounds in assoc type definition (Trait<'a, 'b>::Assoc: 'a + 'b) AND the bound X: Trait<'a, 'a> appears in param env.

bors · 2022-10-12T10:53:12Z

☔ The latest upstream changes (presumably #102948) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2022-10-20T09:27:56Z

☔ The latest upstream changes (presumably #103220) made this pull request unmergeable. Please resolve the merge conflicts.

aliemjay · 2022-11-01T02:45:38Z

r? @compiler-errors who recently touched promoted closure constraints. Feel free to reassign.

bors · 2022-11-04T17:40:04Z

☔ The latest upstream changes (presumably #103962) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2022-11-04T17:52:34Z

This change makes sense to me, and the second commit does have some good diagnostics improvements. r=me after rebasing.

Don't use `ConstraintCategory::ClosureBounds`! Set the category and the span for the promoted constraints to that of the original constraint earlier than before. This eliminates the need for `closure_bounds_mapping`.

Spans are independent of the body being borrow-checked, so they don't need remapping when promoting type-tests and they yield more specific error spans inside bodies of closures/inline consts.

aliemjay · 2022-11-05T05:33:45Z

@bors r=compiler-errors

bors · 2022-11-05T05:33:48Z

@aliemjay: 🔑 Insufficient privileges: Not in reviewers

aliemjay · 2022-11-05T05:35:24Z

Ah no bors delegate command, then ping @compiler-errors to review.

jackh726 · 2022-11-05T17:22:50Z

@bors r=compiler-errors

bors · 2022-11-05T17:22:52Z

📌 Commit 02f78fd has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2022-11-06T05:26:12Z

⌛ Testing commit 02f78fd with merge e30fb6a...

bors · 2022-11-06T09:13:49Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing e30fb6a to master...

rust-timer · 2022-11-06T09:50:30Z

Finished benchmarking commit (e30fb6a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	[-1.4%, -1.2%]	2
Improvements ✅ (secondary)	-3.0%	[-4.0%, -0.2%]	7
All ❌✅ (primary)	-1.3%	[-1.4%, -1.2%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.5%, 0.4%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-2.4%	[-2.6%, -2.0%]	5
All ❌✅ (primary)	-0.4%	[-0.4%, -0.4%]	1

Mark-Simulacrum · 2022-11-08T13:18:45Z

Changes in performance are probably noise given the set of benchmarks.

…=compiler-errors rework applying closure requirements in borrowck Previously the promoted closure constraints were registered under the category `ConstraintCategory::ClosureBounds` in `type_check::prove_closure_bounds()` and then mapped back their original category in `regions_infer::best_blame_constraint` using the complicated map `closure_bounds_mapping`. Now we're registering promoted constraints under their original category and span earlier in `type_check::prove_closure_bounds`. See commit messages. Fixes rust-lang#99245

rust-highfive assigned jackh726 Oct 3, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 3, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2022

aliemjay marked this pull request as ready for review October 3, 2022 12:10

This comment has been minimized.

Sign in to view

aliemjay force-pushed the simplify-closure-promote branch 2 times, most recently from fc12ee6 to f69db63 Compare October 7, 2022 17:58

cjgillot reviewed Oct 8, 2022

View reviewed changes

src/test/ui/generic-associated-types/issue-91139.rs Outdated Show resolved Hide resolved

aliemjay commented Oct 8, 2022

View reviewed changes

aliemjay force-pushed the simplify-closure-promote branch from f69db63 to 4ddd1eb Compare October 13, 2022 06:34

aliemjay force-pushed the simplify-closure-promote branch from 4ddd1eb to 1e5eba9 Compare October 31, 2022 18:14

rustbot assigned compiler-errors and unassigned jackh726 Nov 1, 2022

aliemjay added 2 commits November 5, 2022 07:36

simplify applying closure requirements

df668b9

Don't use `ConstraintCategory::ClosureBounds`! Set the category and the span for the promoted constraints to that of the original constraint earlier than before. This eliminates the need for `closure_bounds_mapping`.

use spans in TypeTest rather than mir::Location

02f78fd

Spans are independent of the body being borrow-checked, so they don't need remapping when promoting type-tests and they yield more specific error spans inside bodies of closures/inline consts.

aliemjay force-pushed the simplify-closure-promote branch from 1e5eba9 to 02f78fd Compare November 5, 2022 04:37

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2022

bors merged commit e30fb6a into rust-lang:master Nov 6, 2022

rustbot added this to the 1.67.0 milestone Nov 6, 2022

bors mentioned this pull request Nov 6, 2022

Don't normalize in AstConv #101947

Merged

aliemjay deleted the simplify-closure-promote branch November 6, 2022 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework applying closure requirements in borrowck #102618

rework applying closure requirements in borrowck #102618

aliemjay commented Oct 3, 2022 •

edited

Loading

rust-highfive commented Oct 3, 2022

This comment has been minimized.

aliemjay Oct 8, 2022

bors commented Oct 12, 2022

bors commented Oct 20, 2022

aliemjay commented Nov 1, 2022

bors commented Nov 4, 2022

compiler-errors commented Nov 4, 2022

aliemjay commented Nov 5, 2022

bors commented Nov 5, 2022

aliemjay commented Nov 5, 2022 •

edited

Loading

jackh726 commented Nov 5, 2022

bors commented Nov 5, 2022

bors commented Nov 6, 2022

bors commented Nov 6, 2022

rust-timer commented Nov 6, 2022

Mark-Simulacrum commented Nov 8, 2022

		\| ^^^^^^^^^^^^^^^^^^^^^^^^^^ closure body requires that `'b` must outlive `'a`
		\| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ argument requires that `'b` must outlive `'a`

rework applying closure requirements in borrowck #102618

rework applying closure requirements in borrowck #102618

Conversation

aliemjay commented Oct 3, 2022 • edited Loading

rust-highfive commented Oct 3, 2022

This comment has been minimized.

aliemjay Oct 8, 2022

Choose a reason for hiding this comment

bors commented Oct 12, 2022

bors commented Oct 20, 2022

aliemjay commented Nov 1, 2022

bors commented Nov 4, 2022

compiler-errors commented Nov 4, 2022

aliemjay commented Nov 5, 2022

bors commented Nov 5, 2022

aliemjay commented Nov 5, 2022 • edited Loading

jackh726 commented Nov 5, 2022

bors commented Nov 5, 2022

bors commented Nov 6, 2022

bors commented Nov 6, 2022

rust-timer commented Nov 6, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Mark-Simulacrum commented Nov 8, 2022

aliemjay commented Oct 3, 2022 •

edited

Loading

aliemjay commented Nov 5, 2022 •

edited

Loading